Conversation
Adds fake_github.go with FakeGitHubClient implementing types.PRRetriever, and github_contract_test.go with a shared runGitHubContractTests function verified against both the fake and the real GitHub API. Documents the V1/V2 behavioural difference for unknown commits: V2 (GraphQL) returns empty with no error; V1 (REST) returns an error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds ProviderAndLabel() to types.PRRetriever, implemented on all four VCS providers (GitHub, GitLab, Azure, Bitbucket) and FakeGitHubClient. Replaces the reflect.TypeOf switch in pullrequest.go with a direct interface call. Adds NewGithubRetrieverFunc factory to internal/github, allowing tests to inject FakeGitHubClient without a real GitHub token. Updates assertPRGithub_test.go and attestPRGithub_test.go to use the fake — KOSLI_GITHUB_TOKEN is no longer required to run these suites. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds ProviderAndLabel() to types.PRRetriever, implemented on all four VCS providers (GitHub, GitLab, Azure, Bitbucket) and FakeGitHubClient. Replaces the reflect.TypeOf switch in pullrequest.go with a direct interface call. Adds NewGithubRetrieverFunc factory to internal/github, allowing tests to inject FakeGitHubClient without a real GitHub token. Updates assertPRGithub_test.go and attestPRGithub_test.go to use the fake — KOSLI_GITHUB_TOKEN is no longer required to run these suites. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
test_smoke_github runs TestGitHubContract_RealGitHub against the real GitHub API (requires KOSLI_GITHUB_TOKEN). test_contract runs both test_smoke_aws and test_smoke_github as a single target for all external service contract tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extends the aws-contract-tests job to use make test_contract, which runs both AWS and GitHub contract tests. Passes KOSLI_GITHUB_TOKEN from secrets. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Claude finished @jumboduck's task in 2m 57s —— View job PR Review: GitHub Fakes Contract Tests
Static AnalysisAll three checks pass cleanly — SummaryThis is a well-structured PR that cleanly extends the fake/contract test pattern from Lambda to GitHub. The design decisions are sound and well-documented in the PR description. What's good:
Minor suggestions (non-blocking):
No bugs or security issues found. The PR is ready to merge with the minor suggestions above at the author's discretion. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Fix variable shadowing: rename provider (string) to providerName in ProviderAndLabel contract test - Move error injection tests inline in TestGitHubContract_Fake with a comment clarifying they test the fake itself, not the contract - Rename CI job key from aws-contract-tests to contract-tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
- Remove stale comment from FakeGitHubClient.PRsByCommit that incorrectly described V1 behaviour (unknown commit returns empty, not an error) - Assert key PREvidence fields in contract tests to match Lambda pattern: URL and State for both V1 and V2; MergeCommit equals the queried commit SHA for V2 (a V2-specific guarantee), non-empty for V1 - Seed MergeCommit in fake test data to support the V2 assertion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ToreMerkely
approved these changes
Apr 22, 2026
Merged
jumboduck
added a commit
that referenced
this pull request
Apr 23, 2026
* chore: rename smoke test targets to contract test targets Rename `test_smoke_aws` → `test_contract_aws` and `test_smoke_github` → `test_contract_github` to use consistent "contract test" terminology throughout the Makefile and TODO.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: add ADR for fakes and contract tests pattern Documents the decision to use in-memory fakes and contract tests to decouple the main integration test suite from live external services, first introduced for AWS Lambda (#763) and extended to GitHub (#807). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: address PR review comments on contract testing ADR - Add 'who can run the tests' as a third problem in Context (Jon's comment) - Clarify that the shared contract function runs against both fake and real adapter — this is the mechanism that guarantees they behave identically (Tooky's comment) - Expand Consequences to reflect the contributor access improvement Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends the fake/contract test pattern (introduced for AWS Lambda in #763) to the GitHub API integration.
Decisions made
Abstraction level — We considered abstracting at the raw SDK level (as Lambda does) but the GitHub integration uses two different clients: a REST client (google/go-github) and a GraphQL
client (shurcooL/graphql). The GraphQL client's reflection-based Query() API makes SDK-level faking awkward. Instead we abstract at the operation level, using the existing
types.PRRetriever interface which already covers both PREvidenceForCommitV1 and PREvidenceForCommitV2.
V1/V2 behavioural difference — The two API versions have genuinely different contracts for unknown commit SHAs: V2 (GraphQL) returns an empty slice with no error; V1 (REST) returns a 422
error. Rather than paper over this, the contract tests and the fake both document and enforce this split. FakeGitHubClient.PREvidenceForCommitV1 returns an error for unseeded commits;
PREvidenceForCommitV2 returns an empty (non-nil) slice.
nil vs empty slice — PREvidenceForCommitV2 on the fake returns []*types.PREvidence{} (not nil) for unknown commits, matching the real implementation's initialised slice. This matters
because nil serialises to JSON null, which the Kosli server rejects.
ProviderAndLabel() on PRRetriever — The command layer used reflect.TypeOf to determine the git provider name and PR label. To allow fake injection without breaking that logic, we added
ProviderAndLabel() (string, string) to types.PRRetriever and removed the reflection entirely. All four VCS implementations (GitHub, GitLab, Azure, Bitbucket) implement the method.
Command-layer injection — A NewGithubRetrieverFunc package-level factory (mirroring NewLambdaClientFunc) allows tests to swap in FakeGitHubClient. The assertPRGithub and attestPRGithub
command test suites no longer require KOSLI_GITHUB_TOKEN.
CI — The daily aws-contract-tests job is replaced by a Contract Tests job running make test_contract, which covers both AWS and GitHub contract tests. KOSLI_GITHUB_TOKEN is already
available in CI secrets.
Test plan